-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix coach, cable tram and trolleybus GTFS route types #6086
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #6086 +/- ##
=============================================
+ Coverage 69.81% 69.86% +0.04%
- Complexity 17418 17437 +19
=============================================
Files 1974 1974
Lines 74545 74547 +2
Branches 7633 7633
=============================================
+ Hits 52047 52080 +33
+ Misses 19848 19828 -20
+ Partials 2650 2639 -11 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to OTP's complexity, I'm afraid there is no such thing as a trivial change anymore and we expect you to write a test for every PR. Luckily, testing this this is quite easy and you can add more test cases to the existing ones.
On top of this, using COACH instead BUS may be the correct thing to do for the 200-299 type range but it is a breaking change so we will need to check with the relevant users.
src/main/java/org/opentripplanner/gtfs/mapping/TransitModeMapper.java
Outdated
Show resolved
Hide resolved
Added change for cable tram as well. GTFS route type 5 was originally called "cable car", but it was a misleading term because a "cable car" means "aerial lift" in Europe, so the specification has now changed to "cable tram". I have changed all references to it in the UI, and also fixed the Transmodel mapping mistake resulted from the misleading term. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment moved to separate lines
…ansmodel mapping of "cableway" to be "GONDOLA"
I am sorry, but I am not in favour of merging this PR, it is possible that we can merge piece of it as separate PRs.
|
Sorry what do you mean? For For bus / coach, do you think that having a feature flag (I propose to name it as |
No one disagree that it was a mistake, but the mistake is adopted in many clients. So, if someone is selling tickets in one of these clients they might filter on CABLE_CAR - even if it is wrong. So to allow them to migrate we need to analyze if it is a problem first, then find a strategy for migration. This is the "real" task of fixing known problems like this one. |
I am going to rework on this and release my proposed changes into a few different packages for each mode of transport. |
Summary
Fix GTFS extended route types for coach (2xx) and trolleybus (8xx). Also change the references of "cable car" in the American sense to "cable tram" according to the reference as "cable car" has a completely separate meaning (aerial lift) in Europe.
Issue
Fixes #5550
Unit tests
added for more existing modes
Documentation
fixed for cable tram